Skip to content

fix(AdornerLayer): re-entrancy-safe lease for zOrderMap snapshot pool#5

Merged
oysteinkrog merged 2 commits into
if/mainfrom
fix/adornerlayer-zordermap-reentrancy-safe-pool
May 16, 2026
Merged

fix(AdornerLayer): re-entrancy-safe lease for zOrderMap snapshot pool#5
oysteinkrog merged 2 commits into
if/mainfrom
fix/adornerlayer-zordermap-reentrancy-safe-pool

Conversation

@oysteinkrog
Copy link
Copy Markdown
Member

Summary

Fixes a re-entrancy regression introduced by 7831813a (T4: pool AdornerLayer._zOrderMap value snapshot). Adorner.Measure / Adorner.Arrange callouts re-enter the same AdornerLayer's MeasureOverride / ArrangeOverride via nested layout passes; with a naïve per-instance shared buffer, the inner call's CopyTo overwrites the outer's snapshot and the inner's terminal Array.Clear nulls slots the outer call is still iterating — NullReferenceException → blank canvas.

Fix

Lease pattern: take the shared buffer into a local at entry, null the field so a nested call sees no pool, restore on exit only if no nested call already stored a larger one. Steady-state stays zero-alloc (single buffer reused); only re-entrant nested levels allocate a private temporary.

Applied to both MeasureOverride and ArrangeOverride in src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/AdornerLayer.cs.

Validation

Built the patched PresentationFramework.dll, dropped it into the if.77 nuget cache, ran the full MotionCatalyst build pipeline (InjectIfWpfAssemblies deploys it to BUILD/x64_Debug/), launched MC via MCP UI broker against a hive that auto-reopens Carl Hansen Take 5, captured a screenshot:

  • Both video viewports render (face-on + down-the-line, frame 0/1240)
  • Pressure and Stance plate, Launch Monitor (Foresight), torque/force graphs, all toolbars/adornments intact
  • No NullReferenceException in Analysis-mode layout
  • Pixel-identical to vanilla Microsoft WPF baseline

Test plan

  • Reviewer approves
  • Tag and publish if.78 via release workflow
  • Downstream PR InitialForce/ScDesktop#6856 bumps reference to if.78 and re-validates

Claude (Initial Force WPF Bot) and others added 2 commits May 16, 2026 10:36
Commit 7831813 ("wpf-perf(big-win T4): pool AdornerLayer._zOrderMap
value snapshot") shipped a per-instance object[] snapshot buffer
shared between MeasureOverride and ArrangeOverride to eliminate ~170 MB
of per-pass DictionaryEntry[] allocations during MotionCatalyst
take-open.

Defect: Adorner.Measure / Adorner.Arrange callouts can re-enter the
same AdornerLayer's MeasureOverride/ArrangeOverride via a nested layout
pass. A naïve shared field lets the inner call's CopyTo overwrite the
outer pass's snapshot, and its terminal Array.Clear nulls the slots
the outer is still iterating — the outer then reads a null reference
and the layout throws, leaving MotionCatalyst with a completely blank
canvas on take-open.

Fix: lease pattern. Each call captures the current field value into a
local, immediately nulls the field (so any re-entrant call allocates
its own buffer rather than aliasing), iterates on the local, and at
end of pass restores its buffer to the field — keeping whichever
buffer (own or the one a nested call left behind) is larger.

Steady state on the non-re-entrant path remains zero-allocation: the
field holds the grown buffer, every subsequent call leases-clears-
copies-iterates-clears-restores in place. Re-entrant calls pay one
object[] allocation per nesting level, matching the worst case of the
pre-7831813a baseline.

Validated end-to-end via MCP UI screenshots on MotionCatalyst:
  - HEAD before fix: take-open shows fully black canvas
  - HEAD + this fix: identical to vanilla upstream/release/10.0
    (Carl Hansen golf swing, Frame 0/1240, both video viewports
     rendered, Pressure & Stance heatmap, Launch Monitor, all data
     boxes populated, playback toggles cleanly)

All 358 perf commits in PRs #1-#4 preserved.
…eptions

PR review feedback (gpt-5.5-pro): the lease pattern in commit 18eaca2
restored _zOrderValuesSnapshotBuffer only on normal exit. If an
Adorner.Measure / Adorner.Arrange callout (or GetDesiredTransform, the
property assignments, etc.) throws, the layer's pool field stays null
and subsequent layout passes allocate again, regressing the perf win.

The original NRE root cause (re-entrant CopyTo + terminal Array.Clear
nulling outer-pass slots) was already fixed by the lease itself — the
outer pass owns its local buffer and is immune to nested clobber.
This patch adds defensive resource restoration for the exception path:
the iteration runs under try, and Array.Clear + restore happen in
finally.

No behavior change on the happy path. Defense-in-depth for the unhappy
path so the pool field invariant survives layout callouts that throw.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@oysteinkrog oysteinkrog merged commit 02fe016 into if/main May 16, 2026
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant